Skip to content

Scheduler - Appointments Refactoring - Rendering#33014

Open
Tucchhaa wants to merge 15 commits intoDevExpress:26_1from
Tucchhaa:new_rendering_26_1
Open

Scheduler - Appointments Refactoring - Rendering#33014
Tucchhaa wants to merge 15 commits intoDevExpress:26_1from
Tucchhaa:new_rendering_26_1

Conversation

@Tucchhaa
Copy link
Copy Markdown
Contributor

No description provided.

@Tucchhaa Tucchhaa self-assigned this Mar 23, 2026
@Tucchhaa Tucchhaa added the 26_1 label Mar 23, 2026
Comment on lines -17 to -18
startDate: Date | string;
endDate: Date | string;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SafeAppointment type actually doesn't have this properties, so I have removed them

@Tucchhaa Tucchhaa marked this pull request as ready for review March 25, 2026 15:16
@Tucchhaa Tucchhaa requested a review from a team as a code owner March 25, 2026 15:16
Copilot AI review requested due to automatic review settings March 25, 2026 15:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new internal Scheduler appointments rendering implementation (behind the _newAppointments option), including incremental rendering via view model diffing, plus supporting utilities, styling updates, and test coverage.

Changes:

  • Added appointments_new components (appointments container, base appointment, grid/agenda appointments, collector) and utilities (diffing, targeted appointment, date text) with Jest tests.
  • Integrated the new appointments implementation into m_scheduler.ts when _newAppointments is enabled.
  • Updated Scheduler appointment CSS selectors and refreshed TestCafe visual etalons accordingly.

Reviewed changes

Copilot reviewed 27 out of 33 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/devextreme/js/__internal/scheduler/utils/get_targeted_appointment.ts Marks old targeted-appointment helper for removal after legacy deletion.
packages/devextreme/js/__internal/scheduler/types.ts Simplifies SafeAppointment typing to align with Scheduler Appointment (Date | string dates already supported).
packages/devextreme/js/__internal/scheduler/m_subscribes.ts Switches legacy date-text formatting to new helper and adds legacy-compat TODOs.
packages/devextreme/js/__internal/scheduler/m_scheduler.ts Wires in new Appointments component behind _newAppointments, including onAppointmentRendered action bridging and container wiring.
packages/devextreme/js/__internal/scheduler/m_compact_appointments_helper.ts Marks legacy helper for removal.
packages/devextreme/js/__internal/scheduler/m_classes.ts Marks legacy class exports for cleanup after old impl removal.
packages/devextreme/js/__internal/scheduler/appointments_new/utils/type_helpers.ts Adds type guards for new appointment view model variants.
packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_view_model_diff.ts Adds diffing logic supporting add/remove/resize operations for incremental rendering.
packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_view_model_diff.test.ts Adds Jest coverage for diffing behavior.
packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_targeted_appointment.ts Adds targeted-appointment construction for new appointment view models (incl. resources).
packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_targeted_appointment.test.ts Adds Jest coverage for targeted appointment construction and resource enrichment.
packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_date_text.ts Adds date text formatting utilities used by new/bridged rendering paths.
packages/devextreme/js/__internal/scheduler/appointments_new/const.ts Introduces shared CSS class constants and localized strings for new appointments rendering.
packages/devextreme/js/__internal/scheduler/appointments_new/appointments.ts New appointments container component implementing diff-based incremental DOM updates.
packages/devextreme/js/__internal/scheduler/appointments_new/appointments.test.ts Jest coverage for rendering, partial updates, all-day container routing, and onAppointmentRendered.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment_collector.ts New appointment-collector component (button-based) for compact/“more” UI.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment_collector.test.ts Jest coverage for collector CSS classes, aria, positioning, and text.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment/grid_appointment.ts New grid appointment component (geometry + content + resource color).
packages/devextreme/js/__internal/scheduler/appointments_new/appointment/grid_appointment.test.ts Jest coverage for grid appointment rendering details and resource coloring.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment/base_appointment.ts Base appointment component with templating, action dispatch, aria role, and common helpers.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment/base_appointment.test.ts Jest coverage for base appointment classes and basic aria role.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment/agenda_appointment.ts New agenda appointment component with marker, details, and resource list rendering.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment/agenda_appointment.test.ts Jest coverage for agenda appointment behavior incl. resources and recurrence.
packages/devextreme/js/__internal/scheduler/appointments_new/mock/appointment_properties.ts Test helpers for generating mock view models and base appointment props.
packages/devextreme-scss/scss/widgets/material/scheduler/_index.scss Updates selectors to apply appointment-content styling without relying on .dx-item-content.
packages/devextreme-scss/scss/widgets/generic/scheduler/_index.scss Same selector adjustment for generic theme.
packages/devextreme-scss/scss/widgets/fluent/scheduler/_index.scss Same selector adjustment for fluent theme.
e2e/testcafe-devextreme/tests/scheduler/common/layout/adaptive/etalons/view=day-crossScrolling=true-vertical-rtl (fluent.blue.light).png Updated visual baseline due to styling/rendering changes.
e2e/testcafe-devextreme/tests/scheduler/common/layout/adaptive/etalons/view=day-crossScrolling=true-rtl (fluent.blue.light).png Updated visual baseline due to styling/rendering changes.
e2e/testcafe-devextreme/tests/scheduler/common/layout/adaptive/etalons/view=day-crossScrolling=false-vertical-rtl (fluent.blue.light).png Updated visual baseline due to styling/rendering changes.

getDataAccessor: () => AppointmentDataAccessor;
}

export class BaseAppointment<
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this in our meeting — in the Scheduler codebase "Appointment" is the source data (what user creates). What we render on the view is an "Occurrence" — we already have the Occurrence type and getOccurrences() method for this.

This is not a nitpick. This is the wrong name. These classes render items on a view, not the source data. Right now we have SafeAppointment, AppointmentItemViewModel, BaseAppointment — three different things all called "Appointment". This makes the code harder to read.

Please rename to BaseOccurrenceView / GridOccurrenceView / AgendaOccurrenceView, or at least AppointmentView.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied new AppointmentView name


export class BaseAppointment<
TProperties extends BaseAppointmentProperties = BaseAppointmentProperties,
> extends DOMComponent<BaseAppointment<TProperties>, TProperties> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at what DOMComponent features are actually used here:

The only place that really needs DOMComponent is _createComponent in AppointmentCollector — it creates a Button and handles disabled/rtl cascading.

But that does not mean every appointment needs to be a DOMComponent. The Collector can get a button factory from the parent instead.

Also: focus() is an empty stub. We lost KBN and accessibility that CollectionWidget gave us for free.

I think each appointment should be a simple object (jQuery element + position + classes), not a full DOMComponent. Template rendering should stay in the parent — like how CollectionWidget does it for List, Gallery, TileView.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point, but I would suggest to postpone removal of DOMComponent. But right now it's a familiar mechanism to pass properties and use common functions.

DOMComponent doesn't introduce a lot of overheads to the appointments and I believe it will be easy to remove DOMComponent in the future.

If you don't mind I would postpone this

import type { GridAppointmentProperties } from './grid_appointment';
import { GridAppointment } from './grid_appointment';

const getGridAppointmentProperties = (options: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test for "show correct title" (line 95-105) needs:

  1. getGridAppointmentProperties()mockGridViewModel() (40 lines, 15+ fields)
  2. getMockedBaseAppointmentProperties() → mocked ResourceManager, DataAccessor
  3. createGridAppointment() → DOMComponent + @ts-expect-error + await process.nextTick

All this to check that a div has text "Test title".

Simpler version:

const $el = renderAppointment({ text: "Test title", startDate, endDate });
expect($el.find(".dx-scheduler-appointment-title").text()).toBe("Test title");

The rendering logic is ~30 lines of jQuery. But we need a 126-line mock file to test it. These tests check how the component is connected (DOMComponent lifecycle, template manager, resource manager) — not what it does (render a title in a div). If AppointmentItemViewModel adds a new field, the mock breaks even though title rendering did not change.

I think this happened because AppointmentItemViewModel is too complex and has too many things inside. It mixes rendering data, positioning, view state, and data access all in one object. If the view model was simpler and focused only on what the component needs to render, the tests would be simpler too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified properties that are passed to the appointments or collector. Now mocks in this tests are simpler.

appointmentDataSource: AppointmentDataSource,
): boolean => {
const updatedAppointmentData = appointmentDataSource.getUpdatedAppointment();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: Just a thought — this is classic LCS with O(n*m) time and space. With virtual scrolling we can have 50-100+ visible appointments, and this runs on every scroll.

Since appointments use absolute positioning (top/left), DOM order does not matter. A simple Map<itemData, component> would give O(n+m) — iterate both lists, find added/removed/kept.

Not critical, you can keep it as is. But think about it — a map-based approach would be simpler and faster here because we do not need to preserve order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about this solution, but we can't use this approach, because recurring appointments have the same itemData, so we don't have a unique key for the appointments


.dx-item-content.dx-scheduler-appointment-content {
.dx-scheduler-appointment-content {
@container (max-height: #{$fluent-scheduler-appointment-25min-height}) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: This removes .dx-item-content from the selector, which changes CSS specificity. This is the only change that affects production (old implementation). The etalon screenshots changed — so there is a visual diff.

This feels out of place in this PR. Maybe move to a separate small PR so we can test it on its own?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if the screenshots changed because of this, but I would like to know if it was because of css or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjbur definitely screenshots changed because of this. But it seems that now it's better, because all day appts have the same padding-right as the common appts.

That happened because of .dx-item-content selector, some styles weren't applied to the all day appts.

@aleksei-semikozov
Copy link
Copy Markdown
Contributor

Good work on isolating the appointment components and the diff-based rendering idea.

I left inline comments, but here is the summary:

1. Feature flag: We are adding ~2800 lines behind _newAppointments that is not enabled and not tested with real Scheduler. This creates 8 if/else branches in m_scheduler.ts.

I think we can do this step by step — replace old code with new code directly, no flag needed:

  • get_date_text.ts can replace m_text_utils.ts + text_utils.ts right now (same logic, cleaner code)
  • get_targeted_appointment.ts can replace the old one in scheduler/utils/
  • AppointmentCollector (107 lines) can replace CompactAppointmentsHelper (205 lines)
  • Then migrate rendering

Each step is small, tested in production, and removes old code instead of adding dead code.

2. No integration tests: 77 unit tests, but no test with a real Scheduler. We do not know if this works end-to-end.

3. DOMComponent per appointment: Each appointment is a full DOMComponent — with template manager, action system, options, lifecycle. But it is just a rectangle with text. This is too much. If we moved away from CollectionWidget, we should use something simpler, not something equally heavy. See inline comments.

Let's discuss the approach before merging.

Copilot AI review requested due to automatic review settings March 27, 2026 13:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 34 changed files in this pull request and generated 5 comments.

Copilot AI review requested due to automatic review settings March 27, 2026 13:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 34 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings March 27, 2026 14:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 34 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

packages/devextreme/js/__internal/scheduler/m_scheduler.ts:998

  • createAppointmentRenderedAction() creates the onAppointmentRendered action without excludeValidators: ['disabled', 'readOnly'], while the legacy path uses that exclusion in getAppointmentRenderedAction(). With _newAppointments enabled, this changes when the handler runs (e.g., it may stop firing when the scheduler is disabled/readOnly). To preserve consistent public event behavior across implementations, pass the same excludeValidators options when creating appointmentRenderedAction.

Comment on lines +16 to +18
export interface SafeAppointment extends Appointment {}

export interface TargetedAppointment extends Appointment {
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SafeAppointment is now just Appointment, which makes startDate / endDate optional again (Appointment allows Date | string | undefined). A lot of internal scheduler code treats SafeAppointment as guaranteed to have these fields (e.g., date math, new Date(appointment.startDate as Date) casts). Consider keeping SafeAppointment as a “normalized” type with required startDate / endDate (and potentially required Date | string), or update internal call sites to safely handle missing dates.

Copilot uses AI. Check for mistakes.
Comment on lines +33 to +86
export interface AppointmentsProperties extends DOMComponentProperties<Appointments> {
currentView: ViewType;
viewModel: AppointmentViewModelPlain[];
items: AppointmentViewModelPlain[]; // TODO: legacy compatibility
$allDayContainer: dxElementWrapper | null;

appointmentTemplate: SchedulerProperties['appointmentTemplate'];
appointmentCollectorTemplate: SchedulerProperties['appointmentCollectorTemplate'];

onAppointmentRendered: BaseAppointmentViewProperties['onAppointmentRendered'];

getAppointmentDataSource: () => AppointmentDataSource;
getResourceManager: () => ResourceManager;
getDataAccessor: () => AppointmentDataAccessor;
}

type AppointmentComponent = GridAppointmentView | AgendaAppointmentView | AppointmentCollector;

export class Appointments extends DOMComponent<Appointments, AppointmentsProperties> {
private appointmentBySortIndex: Record<number, AppointmentComponent> = {};

private get $allDayContainer(): dxElementWrapper | null {
return this.option().$allDayContainer;
}

private get $commonContainer(): dxElementWrapper {
return this.$element();
}

override _init(): void {
super._init();

this._templateManager.addDefaultTemplates({
appointment: new EmptyTemplate(),
appointmentCollector: new EmptyTemplate(),
});
}

override _initMarkup(): void {
super._initMarkup();

this.$element().addClass(APPOINTMENTS_CONTAINER_CLASS);
}

override _getDefaultOptions(): AppointmentsProperties {
return {
...super._getDefaultOptions(),
viewModel: [],
$allDayContainer: null,
appointmentTemplate: 'appointment',
appointmentCollectorTemplate: 'appointmentCollector',
onAppointmentRendered: (): void => {},
};
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppointmentsProperties marks several options as required (currentView, items, and the getter callbacks), but _getDefaultOptions() does not provide defaults for them. This is likely to fail TypeScript checking and also makes this.option().currentView / getResourceManager() etc. potentially undefined at runtime before the component is created with a full config. Consider making these options optional in the interface and guarding usages, or providing safe defaults in _getDefaultOptions().

Copilot uses AI. Check for mistakes.
Comment on lines +141 to +152
private renderAgendaAppointments(appointments: AppointmentViewModelPlain[]): void {
const commonFragment = domAdapter.createDocumentFragment();

this.$commonContainer.empty();

appointments.forEach((appointmentViewModel) => {
const appointment = this.renderAppointment(commonFragment, appointmentViewModel);
this.appointmentBySortIndex[appointmentViewModel.sortedIndex] = appointment;
});

this.$commonContainer.get(0).appendChild(commonFragment);
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renderAgendaAppointments appends new components into appointmentBySortIndex without clearing it first. If the agenda view model shrinks or becomes empty, stale entries remain in the map (potential leaks / wrong lookups later). Reset appointmentBySortIndex (e.g., to {}) before populating it, and handle the empty list case explicitly.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, copilot I think here is right

Copy link
Copy Markdown
Contributor

@aleksei-semikozov aleksei-semikozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


private renderAgendaAppointments(appointments: AppointmentViewModelPlain[]): void {
const commonFragment = domAdapter.createDocumentFragment();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak: appointmentBySortIndex is never reset before re-rendering. Each call to renderAgendaAppointments appends new entries but stale references from previous renders remain. Add this.appointmentBySortIndex = {}; before the forEach.

}

private renderAgendaAppointments(appointments: AppointmentViewModelPlain[]): void {
const commonFragment = domAdapter.createDocumentFragment();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only $commonContainer is cleared here but $allDayContainer is not. If a previous non-agenda render placed elements into $allDayContainer, switching to agenda view will leave those orphaned DOM nodes. Add this.$allDayContainer?.empty(); here as well.

.addClass(APPOINTMENT_CLASSES.CONTENT)
.appendTo(this.$element());

const template = this.option().appointmentTemplate instanceof EmptyTemplate
Copy link
Copy Markdown
Contributor

@aleksei-semikozov aleksei-semikozov Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: instanceof EmptyTemplate check looks fragile. Maybe better to use flag or null check? Not blocker.

);
}

private renderAgendaAppointments(appointments: AppointmentViewModelPlain[]): void {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: No test for grid → agenda switch with allDay appointments. Good to have for cleanup verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants